fix(windows): make recovery evidence authoritative - #3789
Conversation
77dd902 to
b1f348d
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Update on b1f348d66d:
Spec is PASS — recovery probing now uses live fs.promises.open with correct intercept and bounded ADS walk, correctly retrying outside cleanup.
However Standards remains NO-GO:
- Both commits lack required
Generated-by: <tool>trailer despite AI-authored changes (CONTRIBUTING.md:34). - PR template checklist/behavior Yes/No sections are removed, not filled.
Checks on b1f348d66 are test: failure / windows_recovery: failure (both due to queryTurn removal in execution-host-queue.test.ts, not this diff, but still BLOCKED) — not green.
简体中文
实现通过，标准合规与 CI 门待补。
|
Current exact-head failures are upstream bootstrap blockers, not failures in this PR: #3790 prevents Runtime Host build and #3788 prevents message-admission startup. Both are repaired together in green PR #3796 because neither standalone fix can pass required CI before the other lands. After #3796 merges, this branch will rebase, drop the now-upstream schema-constructor overlap, and rerun the automatic windows_recovery check through its real recovery steps. |
b1f348d to
7957ebe
Compare
|
Standards follow-up addressed on new exact head \7957ebe6a: both commits now carry the required \Generated-by: Codex\ trailer, and the PR body restores the Checklist plus behavior Yes/No section. I agree the hosted gates remain blocked by current-main #3788/#3790; their combined repair #3796 is exact-head green and will be merged/rebased before this PR can claim hosted success. |
7957ebe to
9047c2e
Compare
|
Rebased onto merged #3796 at new exact head \9047c2eb3. The overlapping schema-constructor handle patch is now upstream and absent from this diff. Rebase-local planner/harness (76 pass / 0 fail / 1 privilege skip), repository control-plane YAML, lint, format, inventory (64 declarations), Core/Storage builds, and diff check pass. Fresh hosted \ est\ and \windows_recovery\ are running; this head should now reach the actual native recovery steps. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no code blocking issues.
Recovery evidence probing now uses live fs.promises.open with intercept and bounded walk, correctly. No new abstraction.
However machine gate is NO-GO due to branch base drift: execution-host-queue.test.ts:248/249/271 still references removed queryTurn/startTurn/stopTurn — fixed in main by #3796 (c4971c9). Rebase to main ≥ c4971c9 and rerun CI.
Checks on 7957ebe04f are test/windows_recovery: failure due to base drift — not PR-introduced.
简体中文
代码无阻断，基座过期需 rebase。
|
Base-drift follow-up is closed on exact head \9047c2eb3: the PR base is merged #3796 commit \c4971c938, the stale queue aliases and migration collision are inherited from main, and the overlapping schema-constructor patch is absent from this diff. Fresh hosted test passed in 17m28s and windows_recovery passed in 3m23s through all native recovery steps. Both commits retain \Generated-by: Codex; the checklist and behavior declaration are complete. |
9047c2e to
d69a179
Compare
|
@Astro-Han The base-drift blocker you identified is resolved. I rebased onto the current The only textual conflict was in the pull-request workflow allowlist in New exact head: Verification after the rebase:
GitHub now reports the PR as mergeable. The remaining |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Update on d69a179ecb:
Freshness from 7957ebe: rebased with bounded PowerShell+Node walk fallback still fail-closed; marker-open intercept retained. No new P0-P3.
Checks on d69a179ecb5a50976a9652a219d3176ac2f816d7 are test: PASS (32858446128) and windows_recovery: PASS (32858445636).
简体中文
该头仍无新增阻断。
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Review conclusion on exact head d69a179ecb5a50976a9652a219d3176ac2f816d7 (base 421acbeffc65bf47ed05fdc8d2811e15a97246c9): GO with one non-blocking P2; no P0/P1 findings.
The workflow/config relationship is closed: windows_recovery now has stable PR and main triggers, the ASF-managed context list contains the same job name, and the contract tests pin both sides. The marker-race fixture now intercepts the live fs.promises.open implementation, and the PowerShell query fails closed on start, timeout, output-limit, exit, and parse failures. The remaining P2 is an object-coverage gap in that query, noted inline.
Verification: full build:test; focused Storage/marker/crash tests 27 passed with 2 platform skips; CI planner plus release-control tests 62/62; Windows inventory current at 64 declarations; Biome and diff checks clean. Exact-head hosted test and windows_recovery are completed/success, and the hosted merge-result tree equals the reviewed head tree.
简体中文
精确 head `d69a179ecb` 结论：GO，只有 1 条不阻塞的 P2，无 P0/P1。工作流与 required context 的自触发关系、marker 竞态切点及 PowerShell 失败关闭路径均成立；剩余问题是 ADS 查询遗漏目录对象，见行内评论。
ef61719 to
b11da18
Compare
|
P2 follow-up and current gate status on exact head
Hosted |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Update on b11da18e7c:
GO — no P0-P3 on code. Windows recovery now uses P/Invoke FindFirstStreamW with correct handle/EOF handling and directory scanning; prior base compilation P1 closed via rebase. windows_recovery: PASS on this head. Remaining test: FAILURE is unrelated e2e composer-plus-menu-stability flake tracked in #3814, not attributable.
简体中文
该头无阻断，残留失败与本改动无关。
b11da18 to
da660e4
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Update on da660e47:
[P2] Marker authority uses mutable fs.promises.open binding
marker-file.ts:38-42 reads fs.promises.open dynamically per call. Later-loaded code can replace it and forge handles whose stat()/readFile() align with real path — bypasses descriptor/path checks for root identity, weakening flags or causing DoS.
Fix: capture intrinsic at module evaluation or inject explicitly; fixture should patch before dynamic import.
Note: windows_recovery: SUCCESS on this head only ran IPC/SQLite paths, not ADS targeted tests — limited evidence.
简体中文
marker 权威绑定可被替换。
|
@jackwener The directory-ADS P2 is closed on exact head Fresh exact-head gates are green:
Local file/root/nested-directory ADS cases remain 3/3 and managed dependency crash recovery remains 5/5 after the rebase. Could you please re-review the current head when available? |
|
@Astro-Han Fixed in The initialization-race fixture already patches Verification:
|
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Update on 3f046138bd:
CODE GO — marker authority P2 now closed (intrinsic captured at evaluation). No P0-P3 on code.
Delivery NO-GO: hosted windows_recovery: SUCCESS but ADS tests still skipped on Linux (no Windows evidence); test: FAILURE on unrelated packaged-app closure (expects missing @xterm/headless, got extra addon) — gate blocked.
简体中文
代码已通，交付门禁未绿。
|
@Astro-Han The mutable-open P2 is fixed on exact head Fresh hosted gates on this head are both green:
The original ADS directory thread is also resolved, and no unresolved review threads remain. Please re-review this exact head when available. |
16dad9a to
923274b
Compare
|
@Astro-Han Current exact head is The mutable-open P2 is fixed in rebased commit The original ADS directory thread remains resolved, with no unresolved inline review threads. Please re-review this exact head when available. |
|
Current exact head 923274b is fully green on main=8ff9528eb: est 32917151933 and windows_recovery 32917151776. The marker-open P2 is fixed and the original ADS directory thread is resolved; no new issues remain in this PR. |
There was a problem hiding this comment.
I reviewed this head and found no blocking issues.
Only two comment lines changed from 3f04613; production code unchanged and marker authority fix remains. Hosted test: SUCCESS (32917151933) and windows_recovery: SUCCESS (32917151776).
No P0-P3 on code; note Windows ADS targeted tests still not executed on hosted Windows lane (coverage suggestion only).
简体中文
该头无新增阻断。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Review conclusion on exact head 923274b96a68d60eefd5b9d81528c1e28fd4e6fa: GO; I found no blocking issues.
The previous directory-stream finding is closed. The Node walk now submits the dependency root, each non-reparse directory, and each regular file to one bounded query. The PowerShell helper uses FindFirstStreamW / FindNextStreamW, accepts only ERROR_HANDLE_EOF as the no-more-streams result, and fails closed on process, output, parse, and API failures. The new root and nested-directory reopen tests exercise the missing cases. The later marker fix also closes the mutable-open concern by capturing a bound fs.promises.open primitive when the module loads while preserving the intended pre-import test interposition.
I rebuilt the current-main merge result at 45ca7c0c9; it merges cleanly, the full test build passes, and the focused Storage plus repository-control tests pass 154 tests with 7 platform skips and no failures. The exact-head hosted test and windows_recovery jobs are also completed successfully.
One verification boundary remains: windows_recovery does not run managed-dependency-environment.test.js, while the Linux test job skips all three NTFS stream cases. The implementation is supported by static review and the author's reported local Windows run, but the exact-head hosted checks do not independently execute those regressions. This is not a blocking code finding; I recommend adding a focused Windows step with a strict three-pass, zero-skip assertion so a green Windows gate directly proves the behavior it protects.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。
923274b to
f1c5de6
Compare
|
@jackwener Implemented your evidence recommendation in
Fresh exact-head verification on
The original directory-stream thread and the later mutable-open P2 are both addressed; no unresolved inline threads remain. |
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Approved on exact head f1c5de66a5fd1d988a978dd2749bbf20ff658304; I found no blocking issues.
The new commit closes the remaining verification boundary. The hosted Windows job now runs exactly the three NTFS alternate-stream regressions, and its exact-head log records 3 tests, 3 passes, and 0 skips before the other recovery checks. The repository-control test pins the workflow step, test-name pattern, test file, and strict counts, so this evidence cannot silently disappear.
The preceding implementation and marker-authority fixes replay unchanged after the rebase. The branch is based directly on current main, its merge-result tree equals the reviewed head tree, the full test build passes locally, and the focused Storage and repository-control suites pass 156 tests with 7 platform skips and no failures. Hosted test and windows_recovery both completed successfully on this exact head.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查，也不能替代人类审查。有人类对本条评论负责，如有错误请直接指出。
There was a problem hiding this comment.
English
I found one Important issue. Apart from this, I did not find any other blocking or important correctness, marker-race, CI-contract, cross-platform, or simplification concerns.
[Important] Decode the piped path list as UTF-8
child.stdin.end(JSON.stringify(paths)) writes the string as UTF-8, but the PowerShell script only sets $OutputEncoding / Console.OutputEncoding and then reads the request through Console.In.ReadToEnd(). On Windows PowerShell 5.1, Console.In uses Console.InputEncoding, which normally follows the active console/system code page rather than UTF-8.
I reproduced this with the same Node-to-Windows-PowerShell-5.1 pipe on code page 936: C:\Users\张三\node_modules\包 was decoded as C:\Users\寮犱笁\node_modules\鍖?. The JSON remains syntactically valid, so the script reaches FindFirstStreamW with the corrupted path, receives a path-not-found error, and rejects an otherwise valid managed dependency environment. This affects ordinary Windows installations whose user or dependency paths contain non-ASCII characters.
Please decode stdin explicitly as UTF-8 before parsing it. The most self-contained fix is an explicit UTF-8 StreamReader over Console.OpenStandardInput(); it fixes the pipe protocol without relying on or mutating the process-wide console code page. One of the existing Windows ADS tests should also use a non-ASCII storage root or nested entry so the hosted lane covers this boundary.
Reviewed non-finding: I also examined the full-tree paths materialization on both sides of the pipe. It does add O(total path bytes) peak memory for unusually large trees, but the existing acquisition path already has no global entry/resource admission bound, a practical failure requires an extreme tree, and replacing this batch with a correctly cancellable/backpressured streaming protocol would add meaningful lifecycle complexity. I therefore do not consider it a finding or an actionable simplification for this PR.
简体中文
我发现了一个 Important 问题。除此之外，没有发现其他 Blocking 或 Important 级别的正确性、marker 竞态、CI 合同、跨平台或简化问题。
[Important] 应以 UTF-8 解码管道传入的路径列表
child.stdin.end(JSON.stringify(paths)) 会以 UTF-8 写入字符串，但 PowerShell 脚本只设置了 $OutputEncoding / Console.OutputEncoding，随后通过 Console.In.ReadToEnd() 读取请求。Windows PowerShell 5.1 中的 Console.In 使用 Console.InputEncoding，它通常跟随当前控制台或系统代码页，而不是 UTF-8。
我使用相同的 Node 到 Windows PowerShell 5.1 管道在代码页 936 下复现了该问题：C:\Users\张三\node_modules\包 被解码为 C:\Users\寮犱笁\node_modules\鍖?。JSON 在语法上仍然有效，因此脚本会继续使用损坏的路径调用 FindFirstStreamW，得到路径不存在错误，并拒绝原本合法的托管依赖环境。普通 Windows 安装中，只要用户名或依赖路径包含非 ASCII 字符，就可能触发该问题。
请在解析前显式以 UTF-8 解码 stdin。最自包含的修法是在 Console.OpenStandardInput() 上使用显式 UTF-8 的 StreamReader；这样可以直接固定管道协议的编码，不依赖也不修改进程级控制台代码页。还应让一个现有 Windows ADS 测试使用非 ASCII 的存储根目录或嵌套条目，使 hosted Windows lane 覆盖这个边界。
已审查但不构成 finding： 我也检查了管道两端对整棵树 paths 的物化。对于异常大的树，它确实会增加 O(total path bytes) 的峰值内存；但现有 acquisition 路径本来就没有全局 entry/resource admission 上限，实际失败需要极端规模，而且将该批处理替换为可正确取消并处理背压的流式协议，会引入明显的生命周期复杂度。因此，我不认为它是本 PR 的 finding，也不认为它是当前值得实施的简化项。
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
f1c5de6 to
b98f3b3
Compare
|
@M4n5ter Fixed the UTF-8 pipe issue in The Windows PowerShell 5.1 helper now reads Direct evidence:
The hosted |
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
Approved on exact head b98f3b32952c5310dd38bcc49825273480c8e0a2; I found no remaining blocking or important code issues.
The previous UTF-8 stdin finding is closed. The Windows PowerShell 5.1 helper now decodes the pipe through a strict UTF-8 StreamReader, preserving non-ASCII paths and failing closed on invalid input. The updated Windows regression uses a non-ASCII storage root and nested directory, and the exact-head hosted lane records 3 tests, 3 passes, and 0 skips.
The full-tree path materialization remains a reviewed non-finding for this PR. The current red test check is an unrelated Desktop prompt-rail E2E failure; a failed-job rerun has been requested separately.
简体中文
已基于精确 head b98f3b32952c5310dd38bcc49825273480c8e0a2 批准；没有剩余的 Blocking 或 Important 代码问题。
此前的 UTF-8 stdin finding 已关闭。Windows PowerShell 5.1 helper 现在通过严格 UTF-8 的 StreamReader 解码管道输入，既能保留非 ASCII 路径，也会对非法输入 fail closed。更新后的 Windows 回归测试使用了非 ASCII 的存储根目录和嵌套目录；exact-head hosted lane 实际为 3 tests、3 passes、0 skips。
全树路径 materialization 仍是已经审查过但不构成本 PR finding 的事项。当前红色的 test check 来自无关的 Desktop prompt-rail E2E 失败；失败 job 已另外请求重跑。
The recovery lane went back to an unconditional pull request trigger and became a second required context in #3789, which puts a three-minute Windows job on every pull request whether or not it can observe the change. That reverses the automatic-runner-start reduction agreed in #3261, and it does not scale: the platform matrix is heading for four Node versions across Linux, Apple x64, Apple silicon and Windows, where one channel must not gate every other pull request. Give the lane the shape the other conditional Windows lane already uses. A path pre-filter names the storage, runtime and Runtime Host source trees whose crash and owner-death authorities its steps execute, and a nightly run covers the transitive edits a static list cannot match. Drop windows_recovery from the ASF-managed required contexts, because a required context behind a path filter never reports and would freeze every pull request instead. Across the last 30 merged pull requests the filter selects 17 and skips 13. Generated-by: Claude Opus
Three adversarial review rounds converged on the same objection to the previous approach: a paths filter and a required context cannot coexist, so filtering the lane meant giving up the only Windows gate that blocks a merge to main. That trade was not worth making. The filter skipped 12 of the last 30 merged pull requests while the other 18 paid the full three minutes anyway, so it bought roughly a minute of unbilled Windows runner per pull request and paid for it by making every recovery regression advisory. Keep the trigger and the required context exactly as #3789 left them, and move the relevance decision inside the job instead. `scripts/ci-test-plan.mjs` already owns "what does this diff affect" for the core lane, so it gains a windows_recovery selection and the lane consumes it the same way ci.yml consumes its own: one planner step after checkout, then `if:` on every step that installs, builds or verifies. An unrelated diff now costs a checkout and one node invocation instead of three minutes, and the check still reports on every pull request, so a red recovery gate still blocks the merge. Selection is the reverse dependency closure of storage, runtime and Runtime Host, which is why packages/core needs no mention: a core change reaches all three on its own. That closure is also why this replaces rather than supplements a path list. The earlier attempt maintained twenty paths, a tsconfig reference walk and a bespoke YAML scanner, each a second authority next to loadWorkspaceGraph, hasPullRequestTrigger and the planner, and each one of them missed packages/core until review caught it. The contract tests assert the shape this depends on: no paths filter anywhere in the trigger block, every non-checkout step behind the planner output, fetch-depth 0 so the base commit is resolvable, and every planner output consumed by some workflow. All four fail when their subject is reverted. Generated-by: Claude Opus
4 participants
Summary
This PR advances #2624 and the Windows support plan in #2142 by making the native recovery check eligible for branch protection and removing two deterministic failure groups from the Windows baseline.
windows_recoveryon every pull request andmainpush;windows_recoverybesidetestin the ASF-managed required status checks and locks the workflow/config relationship with a CI contract;fs.promises.openintrinsic at module initialization, while the Node 24 Windows initialization-race fixture patches it before dynamically importing the authority;fsutil file queryStreamscall with one bounded Windows stream query fed by Node's no-reparse tree walk;FindFirstStreamW/FindNextStreamW, including Win32 namespaced paths for long-path compatibility;Runtime PTY cleanup is intentionally excluded because another contributor has already claimed that slice on #2624. The remaining Git fixture cleanup and symlink-permission inventory are also not folded into this PR.
Why the ADS change is needed
fsutil file queryStreamsis not a supported subcommand on the current developer Windows build or the GitHub hosted runner. Normal dependency trees therefore failed before their receipt could be verified. The previous ADS test matched the words "alternate data stream" in the query-failure diagnostic, so it passed without proving a named stream had been observed.The replacement keeps traversal in Node, rejects reparse points before the external query, and sends a JSON array over stdin to one bounded Windows PowerShell 5.1 process. The process hosts the documented Win32 stream enumeration APIs because the PowerShell 5.1 provider does not enumerate directory streams. Only
ERROR_HANDLE_EOFis accepted as no streams; other API and process failures reject the environment. Regressions now prove exact named-stream rejection for a regular file, the published dependency root, and a nested directory.Verification
Local Windows, Node 24.19 for platform-sensitive tests:
npm run windows:inventory: current, 64 declarations;git diff --check: passed.Hosted exact-head evidence:
windows_recovery: passed, including the strict UTF-8/non-ASCII managed-dependency ADS gate (3 tests / 3 pass / 0 skip), followed by every recovery step;test: all steps through Runtime Host passed, then the independent prompt-rail virtualization timing issue test(desktop): make prompt-rail tail mounting deterministic #3862 failed Desktop e2e (65 passed, 1 failed, 1 skipped). The separate fix test(desktop): settle prompt-rail scroll jumps #3863 is exact-head green, including the full Desktop e2e suite.The post-merge #3788/#3790 blockers were repaired by merged #3796. The overlapping schema-constructor handle fix is inherited from
mainand no longer appears in this PR diff. The marker P2 is closed by commit917e3d110; the fixture binding clarification is in648585896; the strict ADS gate is in4645cec9c; the UTF-8 pipe fix is inb98f3b329.Administration boundary
.asf.yamlis the repository's declarative branch-protection authority. After merge, ASF infrastructure must apply the updatedmainprotection sowindows_recoveryis actually required. The workflow now reports that stable context on every PR and main push, so enabling it cannot leave unrelated PRs permanently pending.AI use
Tool and scope: Codex traced recent Windows baseline artifacts, reproduced the system API and handle failures on Windows/Node 24, implemented the storage and workflow changes, and ran the listed local gates.
Refs #2624. Refs #2142.
Checklist
windows_recoverypasses on headb98f3b329with strict UTF-8/non-ASCII ADS evidencetestpasses; currently blocked only by independent test(desktop): make prompt-rail tail mounting deterministic #3862, fixed in green test(desktop): settle prompt-rail scroll jumps #3863Does this PR entail a change in behavior?
windows_recoveryreturns to PR/main and becomes a required context; Windows storage validation now uses supported Win32 ADS enumeration for files and directories plus a portable marker-race interception boundary.